@lukasz-zimnoch Can we safely remove it? This function is not used anywhere, but there is some existing local chain implementation, which I'm not sure I can remove.

I think we should be good to remove it. The client reacts to OnDepositSweepProposalSubmitted which pulls past events in the subscription monitoring loop. Moreover, we can check the current lock state in the WalletCoordinator contract with a view function call.

Yeah I think we can remove it.

da06660

As for the functions, it's clear that we want to define them separately for each package, even if it means duplication. But what about the types the functions are expecting or returning?
I don't think duplication is the right way. Alternatively to the current solution where we just import the types from the tbtc package, we could alias them, but it doesn't make much sense.

I'm curious about your opinion @pdyraga @lukasz-zimnoch.

In my opinion, we should look at the packages as some bounded contexts of the same domain:

• pkg/tbtc is the context corresponding to the tbtc wallet
• pkg/coordinator is the context of a tbtc wallet coordinator

That said, both should define domain objects (i.e. types) and functions specific to their contexts. However, some of the domain may be shared between those two contexts and this is the case we observe here. I think we have two options:

1. Declare the shared objects in one of the contexts
2. Extract the shared objects to a separate place

I think option 2 is worse in our case as it will introduce unnecessary confusion. In my opinion, we should aim for option 1 as it is more "natural" for our codebase.

The only question is: "Which package should hold the shared objects?". In our case, the pkg/tbtc is kind of a core package while the pkg/coordinator is kind of an overlay specialized for the coordination part. If we agree about that, this implies that all shared objects should live in the pkg/tbtc package.

I would be fine with the duplication for now but I think the problem with the duplication is that those structs are separate types and the chain implementation struct would have to redeclare the same function multiple times. For example:

func GetDepositRequest(...) coordinator.DepositChainRequest // pkg/coordinator chain interface
func GetDepositRequest(...) spv.DepositChainRequest // pkg/maintainer/spv chain interface
func GetDepositRequest(...) wallet.DepositChainRequest // pkg/maintainer/wallet chain interface

The current solution is the lesser evil but I think we can do better. Instead of creating a dependency to tbtc package, we could have a separate package with the common type declarations.

pkg/chain/types/types.go

pkg/tbtc/chain.go                           // imports pkg/chain/types
pkg/coordinator/chain.go                    // imports pkg/chain/types
pkg/maintainer/wallet/chain.go              // imports pkg/chain/types
pkg/maintainer/spv/chain.go                 // imports pkg/chain/types

EDIT: Alternatively /pkg/chain/tbtc/types/types.go

As an alternative, we could also use a similar pattern that we use in pkg/tecdsa, i.e.:

pkg
    tbtc
        wallet        // holds the current contents of pkg/tbtc
            chain.go  // interfaces and objects specific to wallet context
        coordinator   // holds the current contents of pkg/coordinator
            chain.go  // interfaces and objects specific to coordinator context
        maintainer    // holds the current contents of pkg/maintainer
        chain.go      // interfaces and objects shared by the above sub-packages

Worth noting that all sub-packages can depend on pkg/tbtc. This would correspond to option 1 from my previous comment where we extract the shared domain into a separate place.

I think we intentionally decided to flatten packages compared to pkg/tecdsa and treat pkg/tbtc as the client-side wallet implementation. I don't think the alternative is bad but I also like the fact the packages are currently more flat compared to pkg/tecdsa.

If pkg/tbtc is about the wallet signing logic, the auxiliary maintainer logic shouldn't import it. pkg/tbtc/types is a completely separate package from the Go perspective.

Agreed but in fact, pkg/tbtc is a little bit broader than we assume here. It also contains some logic that is useful for auxiliary packages, for example tbtc.ValidateDepositSweepProposal. That means that even if we extract types to pkg/tbtc/types, we would still have the dependency due to some shared logic. This is why I'm leaning towards leaving shared types in pkg/tbtc and do not complicate the situation with an additional package.

In order to elaborate more on why I'm a bit against pkg/tbtc/types:

• It's a bit too generic and does not give a clear answer about what should live inside and what not
• It is nested within pkg/tbtc which will make a wrong dependency direction: pkg/tbtc -> pkg/tbtc/types. In Go, we typically make sub-packages depending on the outer packages (a lot of examples in Go std lib but also ours pkg/net, pkg/tecdsa, and so on).

If we really insist on extracting, I would rather go with creating pkg/tbtccore and put there all core types and shared logic that does not belong to any specific domain context.

• It is nested within pkg/tbtc which will make a wrong dependency direction: pkg/tbtc -> pkg/tbtc/types. In Go, we typically make sub-packages depending on the outer packages (a lot of examples in Go std lib but also ours pkg/net, pkg/tecdsa, and so on).

Yes, we would have to bend this rule. It is not ideal but we wouldn't be the only ones. For example, the Block structure is defined in github.com/ethereum/go-ethereum/core/types package and it is imported by go-ethereum/core/blockchain_insert.go here. This is exactly our use case. As long as we do it only for the types package, I would be personally fine with it.

• It's a bit too generic and does not give a clear answer about what should live inside and what not

It is less generic than tbtccore :) I see the reasoning behind the tbtccore but I am afraid this will sooner or later evolve into a common bag of stuff and create even more complicates mesh of dependencies. I think it's nothing wrong if coordinator or maintainer packages imports logic from tbtc. It is clear, they perform the same validation as the wallet as in the mentioned tbtc.ValidateDepositSweepProposal example and in my opinion, this is a really nice feature of the current code organization.

My slight preference is for a separate types package with good documentation of why it's there and what types should be there. This would give us more flexibility and avoid import cycles. But I am also fine with leaving it as-is with imports from tbtc.

Unfortunately, it seems that there is no perfect solution.

If we decide to declare the types in a common types package what types should go there? All the types used by chain interfaces across all the packages? We have types that are common to pkg/tbtc and pkg/coordinator and specific to the pkg/tbtc only. Which do we want to move to the common types?

Separating types from the chain interfaces declaration may be confusing, as in V2 we decided to declare the chain interfaces closer to the actual usage in packages.

Importing types defined in pkg/tbtc package into the pkg/coordinator doesn't seem perfect either. The pkg/coordinator package uses pkg/tbtc types, but also declares its' own types (e.g. NewWalletRegisteredEvent).

We cannot duplicate the types either, as the functions defined in different packages will require the same type, e.g. ValidateRedemptionProposal defined in both pkg/coordinator and pkg/tbtc must use the same RedemptionProposal type.

I lean towards the current approach where types are defined in pkg/tbtc which is considered a broader and core package and we just import them in pkg/coordinator. This isn't perfect but it doesn't require an aggressive refactor.

We could add another issue for the future to find the perfect solution for types definition.

Captured it here: #3652

Yeah I think we can remove it.

So far, the structure of interfaces in the chain.go file reflected the contract structure we have on the chain. I think we should stick to that approach and avoid creating interfaces like ValidateDepositSweepProposalChain. If we go that way, we would soon have problems with defining the right boundaries between those interfaces. For example, ValidateDepositSweepProposalChain declares GetPendingRedemptionRequest. What if I need this function for a purpose other than proposal validation?

For example, ValidateDepositSweepProposalChain declares GetPendingRedemptionRequest. What if I need this function for a purpose other than proposal validation?

This is the case for #3649 currently in progress.

I do agree with this comment. I see it was done to avoid the duplication here:

but I think having the duplication of function declarations is a better choice in this particular case. I am not a huge fan of importing types from pkg/tbtc as you already know from the other comment but importing interfaces makes it look even worse for me. 😉

Alternative approach: 70bd449

This approach is fine for me but I'd like to hear from @lukasz-zimnoch if it does not break anything for the automation part.

Looks good!

I realized we have a neat pattern we agreed on early in pkg/sortition where we define the local chain implementation in pkg/internal/local. This way, _test.go is only for actual tests and we can unit-test the local chain implementation as well. I know we were not very consistent in using this pattern, but this refactor feels like a good opportunity to align at least this package.

cc @lukasz-zimnoch @tomaszslabon

This pattern can cause import cycles. Suppose we implement tbtc.Chain in pkg/tbtc/internal/local.go, then the structure declared in this internal package must refer to tbtc chain types used by tbtc.Chain. On the other hand, the unit tests living in tbtc package must create an instance of local.Chain. Here is why this pattern is used only in pkg/sortition where sorition.Chain does not declare any chain types.

... and that's why I think we should have the types declared in a separate package 😁

I know we were not very consistent in using this pattern, but this refactor feels like a good opportunity to align at least this package.

IMHO if agree on a specific pattern we should refactor and apply it to the whole keep-core project. Currently, we have multiple patterns that we use randomly in different packages. When we create a new package we're not sure which pattern to follow, and we add even more inconsistencies.

Are you proposing to do the all-in-one refactor in this PR?

Since it doesn't look like we have a consistent idea of the approach we want to follow, and it seems out-of-scope of the current PR and #3632, I propose to handle a refactor of all local chain implementations in a separate PR (along with other changes like one mutex for the local chain you proposed recently).

Captured it here: #3652